Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[vcpkg ci] error out cmake related deprecated functions #31080

Merged

Conversation

JackBoosY
Copy link
Contributor

Throw error message when cmake-related deprecated function is detected in the changes.

Related: #30070

@JackBoosY
Copy link
Contributor Author

Okay I need to pick another var name.

@Adela0814 Adela0814 added category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly category:infrastructure Pertaining to the CI/Testing infrastrucutre and removed category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly labels Apr 24, 2023
@Adela0814
Copy link
Contributor

Thanks for your PR.
Sometimes additional problems arise after deprecated function changes, which may affect the merge of high-priority fixes. Our guidebook says to avoid using deprecated functions, not prohibit. @BillyONeal Please help to confirm whether we need to error out the deprecated function?

@BillyONeal
Copy link
Member

Sometimes additional problems arise after deprecated function changes, which may affect the merge of high-priority fixes. Our guidebook says to avoid using deprecated functions, not prohibit. @BillyONeal Please help to confirm whether we need to error out the deprecated function?

The maintainer guidelines describe it as a suggestion because we did not want to force people to make unrelated changes for simple version updates and similar. There was still a lot of use of the deprecated function in the curated registry, and forcing people who aren't interested in overall 'cleanup' of code they don't own in our registry makes it less nice to be a contributor.

Now that there are no calls to the deprecated functions left in the tree, I think we can block it entirely.

Note that CI doesn't provide a means of testing this change because it only runs some of these scripts from master rather than from the PR branch.

Adela0814
Adela0814 previously approved these changes Apr 25, 2023
@Adela0814 Adela0814 added the info:reviewed Pull Request changes follow basic guidelines label Apr 25, 2023
@dg0yt
Copy link
Contributor

dg0yt commented Apr 25, 2023

Note that CI doesn't provide a means of testing this change because it only runs some of these scripts from master rather than from the PR branch.

@BillyONeal Can't you trigger a manual run in the vcpkg.ci pipeline (no parent hashes, no caching)?

@BillyONeal
Copy link
Member

@BillyONeal Can't you trigger a manual run in the vcpkg.ci pipeline (no parent hashes, no caching)?

I can trigger manual runs of the Azure Pipelines stuff against forks, but not Actions stuff against forks. (Because Actions can write-back to the repo and Pipelines can't, Actions only ever runs trustedPR.yml from master, not from the fork)

To meaningfully test changes to the Actions stuff one needs to setup Actions in their own fork with main pointing to the base commit SHA of the PR, and then make PRs into it intentionally triggering problems to show that they are problems.

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This causes the bot to break. See test cases:

@BillyONeal BillyONeal added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Apr 27, 2023
@Adela0814
Copy link
Contributor

Adela0814 commented May 5, 2023

@JackBoosY Note: I will be converting your PR to draft status. Please reactivate this PR once it is ready for review. Thanks.

@Adela0814 Adela0814 marked this pull request as draft May 5, 2023 08:12
@JackBoosY JackBoosY marked this pull request as ready for review May 8, 2023 02:51
@JackBoosY
Copy link
Contributor Author

@BillyONeal Please retest CI scripts.

@Adela0814 Adela0814 added the info:reviewed Pull Request changes follow basic guidelines label May 22, 2023
@dan-shaw
Copy link
Contributor

Can you test this on your repository with an Actions pipeline? See Billy's comment on how to do so

@JackBoosY
Copy link
Contributor Author

JackBoosY commented May 29, 2023

@dan-shaw dan-shaw merged commit 8b10402 into microsoft:master May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:infrastructure Pertaining to the CI/Testing infrastrucutre info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants